ref(autofix): Split explorer response into sections#110527
Conversation
Directly extracting the artifacts makes it hard to handle the loading states. This refactors it so the response is split into sections according to the step, then we find the last matching artifact from the step.
github-actions
bot
added
the
Scope: Frontend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| {sections.map(section => { | ||
| if (isRootCauseSection(section)) { | ||
| return <RootCauseCard key={section.step} section={section} />; |
There was a problem hiding this comment.
Bug: Re-running an autofix step creates components with duplicate React key props because section.step is used as the key, leading to potential rendering issues.
Severity: MEDIUM
Suggested Fix
To ensure stable and unique keys for React's reconciliation algorithm, generate a unique identifier for each section when it is created. This unique ID should be used as the key prop instead of the non-unique section.step string.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/components/events/autofix/v3/content.tsx#L65-L67
Potential issue: When a user re-runs an autofix step, such as 'root_cause', the
`getOrderedAutofixSections` function creates a new section object. Both the old and new
sections for that step will have the same `step` name (e.g., 'root_cause'). In
`content.tsx`, the code maps over these sections and uses `section.step` as the React
`key` prop. This results in multiple components in the list having the same key, which
can lead to unpredictable UI behavior, incorrect component state, and rendering issues
in React.
There was a problem hiding this comment.
expectation is only 1 of each step exists
| } | ||
|
|
||
| if (isArrayOf(artifact, isRepoPRState) && artifact.length) { | ||
| return <PullRequestsCard key="pull-requests" artifact={artifact} />; |
There was a problem hiding this comment.
Duplicate React keys when sections share step names
Medium Severity
Sections are rendered using key={section.step}, but getOrderedAutofixSections can produce multiple sections with the same step value (e.g., when a step is retried via the "No" button calling startStep with the same step name, the backend may append new blocks with a repeated metadata.step). Duplicate React keys cause incorrect DOM reuse and rendering glitches. Consider using the section's index or a unique identifier as the key instead.
Additional Locations (1)
There was a problem hiding this comment.
expectation is only 1 of each step exists
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| } | ||
|
|
||
| section.messages.push(message); | ||
| section.artifacts.push(...(block.artifacts ?? [])); |
There was a problem hiding this comment.
Patches collected before prior section is finalized
Low Severity
In the loop inside getOrderedAutofixSections, each block's merged_file_patches are added to mergedByFile before finalizeSection() is called for the previous section. If a block that starts a new section also carries merged_file_patches, those patches leak into the previous code_changes section's artifact snapshot. The patch collection (lines 346–351) needs to happen after the finalization check (lines 356–365) to ensure the outgoing section receives only patches from its own blocks.
| export function getOrderedAutofixArtifacts( | ||
| runState: ExplorerAutofixState | null | ||
| ): AutofixArtifact[] { | ||
| export function getOrderedAutofixSections(runState: ExplorerAutofixState | null) { |
There was a problem hiding this comment.
do you mind adding some comments on the operations being done in this function? I found the imperative style of updating section and pushing to sections a little confusing to read, though makes sense now
| OrderedArtifact | OrderedExplorerFilePatch | ||
| >(); | ||
| const mergedByFile = new Map<string, ExplorerFilePatch>(); | ||
| function finalizeSection() { |
There was a problem hiding this comment.
can we add a comment on what this fx does
2 participants
Directly extracting the artifacts makes it hard to handle the loading states. This refactors it so the response is split into sections according to the step, then we find the last matching artifact from the step.